Repository navigation
Fix RandomSampleImputer and DropMissingData on non-pandas backends - #1024
Merged
solegalli merged 1 commit intoAug 30, 2026
Merged
Conversation
solegalli
force-pushed
the
narwhals-fix-random-sample-and-drop-missing
branch
2 times, most recently
from
August 30, 2026 18:20
bb89371 to
b58be49
Compare
RandomSampleImputer._transform_narwhals had two bugs on the polars/narwhals path: - it wrote each variable's imputation into a fresh copy of the input (`nw_X = X.with_columns(...)`), so only the last imputed variable survived and every earlier variable kept its nulls; it also returned a narwhals frame instead of a native one. - the "observation" seed branch read `nw_X` before it was ever assigned, raising UnboundLocalError. Both branches now accumulate into `X` and the method returns `X.to_native()`, matching the pandas branch. TransformXyMixin.transform_x_y still assumed check_X_y returned a native dataframe. Since check_X_y now returns a narwhals frame, `is_pandas_dataframe` was always False, so pandas input took the positional-backend path and the `__feature_engine_row_index__` tag column made transform() fail the column-count check. The mixin now branches on `implementation.is_pandas()`, and `_check_X_matches_training_df` ignores the reserved tag column (its name is now a shared constant in dataframe_checks). Fixes the polars cases of test_random_sample_imputer.py and both cases of test_drop_missing_data.py::test_transform_x_y. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
solegalli
force-pushed
the
narwhals-fix-random-sample-and-drop-missing
branch
from
August 30, 2026 18:23
b58be49 to
a6ac9dd
Compare
solegalli
added a commit
that referenced
this pull request
Sep 14, 2026
_check_transform_input_and_state() now returns the narwhals frame, so filter it directly instead of re-wrapping it with nw.from_native(). TransformXyMixin.transform_x_y() (rebased onto #1024) widens n_features_in_ for the row-index tag column; also widen feature_names_in_, since BaseOutlier reorders X to feature_names_in_ and would otherwise drop the tag column on non-pandas backends. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
solegalli
added a commit
that referenced
this pull request
Sep 15, 2026
_check_transform_input_and_state() now returns the narwhals frame, so filter it directly instead of re-wrapping it with nw.from_native(). TransformXyMixin.transform_x_y() (rebased onto #1024) widens n_features_in_ for the row-index tag column; also widen feature_names_in_, since BaseOutlier reorders X to feature_names_in_ and would otherwise drop the tag column on non-pandas backends. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
solegalli
added a commit
that referenced
this pull request
Sep 19, 2026
_check_transform_input_and_state() now returns the narwhals frame, so filter it directly instead of re-wrapping it with nw.from_native(). TransformXyMixin.transform_x_y() (rebased onto #1024) widens n_features_in_ for the row-index tag column; also widen feature_names_in_, since BaseOutlier reorders X to feature_names_in_ and would otherwise drop the tag column on non-pandas backends. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
solegalli
added a commit
that referenced
this pull request
Sep 19, 2026
_check_transform_input_and_state() now returns the narwhals frame, so filter it directly instead of re-wrapping it with nw.from_native(). TransformXyMixin.transform_x_y() (rebased onto #1024) widens n_features_in_ for the row-index tag column; also widen feature_names_in_, since BaseOutlier reorders X to feature_names_in_ and would otherwise drop the tag column on non-pandas backends. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
solegalli
added a commit
that referenced
this pull request
Sep 19, 2026
_check_transform_input_and_state() now returns the narwhals frame, so filter it directly instead of re-wrapping it with nw.from_native(). TransformXyMixin.transform_x_y() (rebased onto #1024) widens n_features_in_ for the row-index tag column; also widen feature_names_in_, since BaseOutlier reorders X to feature_names_in_ and would otherwise drop the tag column on non-pandas backends. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
solegalli
added a commit
that referenced
this pull request
Sep 19, 2026
* Migrate OutlierTrimmer to narwhals, add polars support transform() now filters rows via a single narwhals .filter() call built from a combined boolean expression (AND of each variable's right/left cap conditions), instead of a pandas .loc masking loop. Benchmarked against pandas-native and a numpy boolean-mask extraction at 10k/50k/ 100k rows x 1/2/10 columns: the narwhals filter is within 1.4-1.75x of pandas-native at 10k rows (sub-millisecond absolute difference) and becomes faster than pandas-native from 50k rows up (0.58x-0.97x), so a single merged code path (no is_pandas branching) is the right call here - unlike BaseOutlier's elementwise capping, which benefits from numpy grouping, row-filtering is exactly what narwhals .filter() already pushes down to the native backend efficiently. Also fixes a latent bug in TransformXyMixin.transform_x_y() (_base_transformers/mixins.py): the non-pandas branch added a row-index marker column via with_row_index() and passed it straight to self.transform(), but never widened feature_names_in_/ n_features_in_ to account for it. Any transform() that validates column count (BaseOutlier._check_transform_input_and_state, via _check_X_matches_training_df) then raised a ValueError on the extra column. This was latent because no narwhals-migrated class on this branch previously combined TransformXyMixin with a column-count- checking transform() on a non-pandas backend - OutlierTrimmer is the first. The fix (guarded widen/restore of feature_names_in_ around the transform() call) is carried over verbatim from the same fix already applied to this file on branch narwhals-drop-missing-data (commit fd99caf), which hadn't been merged into this branch yet. Tests rewritten to one parametrized test per behavior over make_df in [pd.DataFrame, pl.DataFrame], plus a new test asserting that caps on two different variables combine with AND (each variable drops a distinct row) - a code path the old sequential-loop version exercised implicitly but no test isolated directly. Docs verified against live output: the class docstring's pandas examples were already accurate; the user guide's Titanic-based numbers had drifted from the current openml dataset (predates this migration, e.g. the IQR section's age max was already wrong against the old pandas-loop transform()) and are corrected here, plus a "With polars" section is added. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Adapt OutlierTrimmer to narwhals-returning check_X _check_transform_input_and_state() now returns the narwhals frame, so filter it directly instead of re-wrapping it with nw.from_native(). TransformXyMixin.transform_x_y() (rebased onto #1024) widens n_features_in_ for the row-index tag column; also widen feature_names_in_, since BaseOutlier reorders X to feature_names_in_ and would otherwise drop the tag column on non-pandas backends. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Use shared backend test fixtures and helpers in OutlierTrimmer tests Replace the file-local data dicts and _cols/_to_list/_make_series helpers with the shared test structure: make_df and data_normal_dist / data_na fixtures, y built with make_series, isinstance(X, make_df) plus to_dict() checks, and pytest.raises(match=...). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Use frame_to_dict after the shared helper rename in #1045 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Document and test infinite caps for variables without variation Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Align OutlierTrimmer and its tests with the repo conventions Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Keep transform_x_y row alignment in OutlierTrimmer instead of the shared mixin Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
RandomSampleImputer._transform_narwhals had two bugs on the polars/narwhals path:
nw_X = X.with_columns(...)), so only the last imputed variable survived and every earlier variable kept its nulls; it also returned a narwhals frame instead of a native one.nw_Xbefore it was ever assigned, raising UnboundLocalError. Both branches now accumulate intoXand the method returnsX.to_native(), matching the pandas branch.TransformXyMixin.transform_x_y still assumed check_X_y returned a native dataframe. Since check_X_y now returns a narwhals frame,
is_pandas_dataframewas always False, so pandas input took the positional-backend path and the__feature_engine_row_index__tag column made transform() fail the column-count check. The mixin now branches onimplementation.is_pandas(), and_check_X_matches_training_dfignores the reserved tag column (its name is now a shared constant in dataframe_checks).Fixes the polars cases of test_random_sample_imputer.py and both cases of test_drop_missing_data.py::test_transform_x_y.